Skip to content

feat: add version field to ExecutionContext#6052

Merged
alamb merged 3 commits into
apache:mainfrom
Weijun-H:add-version-field-to-ExecutionContext
Apr 24, 2023
Merged

feat: add version field to ExecutionContext#6052
alamb merged 3 commits into
apache:mainfrom
Weijun-H:add-version-field-to-ExecutionContext

Conversation

@Weijun-H

Copy link
Copy Markdown
Member

Which issue does this PR close?

Closes #1517

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions Bot added the core Core DataFusion crate label Apr 18, 2023

@alamb alamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Weijun-H

I realize #1517 says "ExecutionContext" but this code is in the experimental "scheduler"

I wonder if #1517 meant to say SessionContext or SessionState or TaskContext (or perhaps the code has been changed since the ticket was filed)

Comment thread datafusion/core/src/scheduler/task.rs Outdated

/// The shared state of all [`Task`] created from the same [`PipelinePlan`]
#[derive(Debug)]
#[allow(dead_code)]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about adding a function like

pub fn version(&self) -> &str {
...
}

Which would avoid the need for allow(dead_code) perhaps

@Weijun-H

Copy link
Copy Markdown
Member Author

Thanks @Weijun-H

I realize #1517 says "ExecutionContext" but this code is in the experimental "scheduler"

I wonder if #1517 meant to say SessionContext or SessionState or TaskContext (or perhaps the code has been changed since the ticket was filed)

I vote for SessionState, because it is the execution context for executing queries. What do you think? 🤔

@Weijun-H Weijun-H force-pushed the add-version-field-to-ExecutionContext branch from f8d81a0 to 8b2154d Compare April 20, 2023 21:12

@alamb alamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Weijun-H !


/// Return version
pub fn version(&self) -> &str {
&self.version

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could just call env!("CARGO_PKG_VERSION") here -- is there any reason to create a string to do so?

Suggested change
&self.version
env!("CARGO_PKG_VERSION")

@alamb alamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @Weijun-H 👍

@alamb alamb merged commit 9775305 into apache:main Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add version field to ExecutionContext

2 participants